Docs: Add documentation for WordPress.PHP.TypeCasts#2591
Docs: Add documentation for WordPress.PHP.TypeCasts#2591brentwilson-clariio wants to merge 3 commits into
Conversation
jrfnl
left a comment
There was a problem hiding this comment.
@brentwilson-clariio Thanks for picking this up! I've just had a quick look and left some feedback for you to consider.
| Note: Other type-cast rules in the WordPress standard | ||
| are enforced by different sniffs: | ||
| - Short forms like (int), (bool), (string): | ||
| PSR12.Keywords.ShortFormTypeKeywords | ||
| - No spaces inside parentheses: | ||
| Squiz.WhiteSpace.CastSpacing | ||
| - Exactly one space after the cast: | ||
| Generic.Formatting.SpaceAfterCast |
There was a problem hiding this comment.
This whole block doesn't belong here. Each sniff functions independently of other sniffs. That also means that the docs should be independent of each other and should just focus on what this sniff does.
There was a problem hiding this comment.
Thanks! I’ve removed the paragraph referencing other sniffs. The docs now only describe what WordPress.PHP.TypeCasts enforces.
| <code_comparison> | ||
| <code title="Invalid: legacy float casts"> | ||
| <![CDATA[ | ||
| <?php |
There was a problem hiding this comment.
PHP open tag is not needed here. (Same for the "invalid" code sample)
There was a problem hiding this comment.
Removed <?php from all code samples as suggested.
| <documentation title="Type Casts"> | ||
| <standard> | ||
| <![CDATA[ | ||
| This sniff normalizes float type cast keywords. |
There was a problem hiding this comment.
Reworded the opener to be shorter and clearer. It now states the rules enforced without repeating itself.
| @@ -0,0 +1,38 @@ | |||
| <?xml version="1.0"?> | |||
| <documentation title="Type Casts"> | |||
There was a problem hiding this comment.
| <documentation title="Type Casts"> | |
| <?xml version="1.0"?> | |
| <documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | |
| xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd" | |
| title="Type Casts" | |
| > |
There was a problem hiding this comment.
Added the schema header and xsi:noNamespaceSchemaLocation pointing to phpcsdocs.xsd.
| Use (float) instead of legacy synonyms: | ||
| - Disallowed: (double), (real) | ||
| - Allowed: (float) |
There was a problem hiding this comment.
This is incorrect/incomplete.
The sniff:
- Forbids the use of non-standard casts for floats.
- Forbids the use of the
(unset)cast. - Discourages the use of the
(binary)cast.
Considering the above, I believe the code samples also need work.
There was a problem hiding this comment.
Updated the standard text and the code examples to cover all three behaviors:
• (double)/(real) → error (normalize to (float))
• (unset) → error (removed in PHP 8.0; suggest unset())
• (binary) → warning (discouraged)
| </standard> | ||
|
|
||
| <code_comparison> | ||
| <code title="Invalid: legacy float casts"> |
There was a problem hiding this comment.
| <code title="Invalid: legacy float casts"> | |
| <code title="Invalid: Using legacy float casts."> |
There was a problem hiding this comment.
Adjusted the code block titles to your suggested wording.
| $size = <em>(real)</em> $value; | ||
| ]]> | ||
| </code> | ||
| <code title="Valid: normalized float cast"> |
There was a problem hiding this comment.
| <code title="Valid: normalized float cast"> | |
| <code title="Valid: Using normalized float casts."> |
There was a problem hiding this comment.
Adjusted the code block titles to your suggested wording.
There was a problem hiding this comment.
Updated code block titles to “Valid: Using normalized float casts.” (and similar phrasing for the other examples), as suggested.
| ]]> | ||
| </standard> | ||
|
|
||
| <code_comparison> |
There was a problem hiding this comment.
The code comparison should have "valid" on the left (first code sample), "invalid" on the right (second code sample).
There was a problem hiding this comment.
Thanks for the feedback! I’ve reordered the code examples so that Valid is on the left and Invalid is on the right, as suggested.
…set), discourage (binary))
rodrigoprimo
left a comment
There was a problem hiding this comment.
Thanks for this PR, @brentwilson-clariio! I'm genuinely sorry for the delay to respond. There was some miscommunication and I didn't realize the WPCS maintainers were waiting for me to post a review. I will make sure to respond faster going forward.
I checked and confirmed that you addressed the points that Juliette raised. I left a few other remarks for us to discuss. Let me know what you think and if you have any questions.
Thanks again.
| <documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd" | ||
| title="Type Casts" | ||
| > |
| </code> | ||
| <code title="Invalid: Using the (unset) cast."> | ||
| <![CDATA[ | ||
| result = <em>(unset)</em> $value; |
There was a problem hiding this comment.
| result = <em>(unset)</em> $value; | |
| $result = <em>(unset)</em> $value; |
bytes below is also missing a $.
| Enforce normalized and safe type casts. | ||
|
|
||
| • Use (float) instead of legacy float casts. | ||
| • Do not use the (unset) cast. Use unset(). | ||
| • Avoid the (binary) cast (discouraged). |
There was a problem hiding this comment.
| Enforce normalized and safe type casts. | |
| • Use (float) instead of legacy float casts. | |
| • Do not use the (unset) cast. Use unset(). | |
| • Avoid the (binary) cast (discouraged). | |
| Enforce normalized and safe type casts. | |
| • Use (float) instead of legacy float casts. | |
| • Do not use the (unset) cast. Use unset(). | |
| • Avoid the (binary) cast (discouraged). |
| </code_comparison> | ||
|
|
||
| <code_comparison> | ||
| <code title="Valid: Prefer a clear alternative."> |
There was a problem hiding this comment.
Maybe "Valid: Using (string) cast instead."? This way the title clearly communicates what is the alternative like the other titles.
That being said, I noticed that the sniff also detects the binary string prefix notation b"string" which is also tokenized as T_BINARY_CAST, but in this case using (string) is not the correct alternative. I believe we should explicitly mention in the documentation that b"string" also trigger the sniff both here with an example and in the standard description above.
In that case, the title could be something like "Valid: (binary) is not used." or "Valid: Use (string) or regular strings."
Another option is to create a separate <code_comparison> block for the binary string. Tipically, sniff documentation uses a single <code_comparison> block per warning/error message that is triggered by the sniff. But in this case it might make sense to have a separate block.
What do you think?
| <code_comparison> | ||
| <code title="Valid: Prefer a clear alternative."> | ||
| <![CDATA[ | ||
| bytes = (string) $value; |
There was a problem hiding this comment.
| bytes = (string) $value; | |
| bytes = <em>(string)</em> $value; |
| > | ||
| <standard> | ||
| <![CDATA[ | ||
| Enforce normalized and safe type casts. |
There was a problem hiding this comment.
I'm unsure about a few things in this paragraph:
- Mentioning that the casts that are not recommended are not safe. Are they all really not safe? I can see why using the casts that were removed is not safe, but not all casts discouraged by the sniff were removed.
- Maybe we could explicitly mention the legacy float casts?
- I don't think it is necessary to mention in parenthesis that
(binary)is discouraged. If we want to mention that maybe we can include it in the main sentence?
Here is a suggestion of a possible alternative for description. Feel free to create your own version.
Type casts must use normalized keywords.
• Use (float) not (double) or (real).
• Do not use the (unset) cast. Use unset().
• Do not use the (binary) cast or b"string" prefix. Use (string) casts or regular string literals instead.
| <code_comparison> | ||
| <code title="Valid: Use unset()."> | ||
| <![CDATA[ | ||
| unset( $value ); |
There was a problem hiding this comment.
| unset( $value ); | |
| <em>unset</em>( $value ); |
|
I just realized something else. From php.net (https://www.php.net/manual/en/language.types.type-juggling.php):
Since the manual says that there is no guarantee that (binary) and (string) will continue to be identical, I guess we should not recommend it in the documentation. Instead maybe the documentation should say that using (binary) is not recommended without providing an alternative? That may be the reason the sniff message does not recommend using (string). |
Just so you know: the |
|
Thanks for your input, @jrfnl. In that case, I think that @brentwilson-clariio can ignore my last comment and the documentation can mention that |
|
@brentwilson-clariio , I was just wondering if you'll have a chance to finish this off in the near future. It would be great if this PR could be included in the next WPCS release. If you haven't got time or lost interest, please let us know, and we'll see if we can find someone to take over. Thanks! |
|
@rodrigoprimo Let me see if I can circle back to this in the next week or two to get it finished up. |
|
@brentwilson-clariio, please let us know within a week if you are still interested in finishing this PR. If we don't hear back from you, we will presume you don't have time, and we will see if we can find someone else to take over and finish it. Thanks for your work so far! |
|
@rodrigoprimo Sorry for the delay. Please feel free to pass this task along to someone else. |
|
No worries and thanks for the work you put into this, @brentwilson-clariio! |
Related to #1722
This PR adds a new documentation file:
WordPress/Docs/PHP/TypeCastsStandard.xmlWhat it covers
Documents the rule enforced by the
WordPress.PHP.TypeCastssniff:(double),(real)) to(float).Notes
Other type-cast related rules in the WordPress standard are enforced by separate sniffs:
(int),(bool),(string)): PSR12.Keywords.ShortFormTypeKeywords